Replace Chart.js default tooltip with a custom Chakra UI tooltip #55994
Conversation
guan404ming
requested review from
bbovenzi,
jscheffl,
pierrejeambrun,
ryanahamilton and
shubhamraj-git
as code owners
|
I am starting to feel like relying on chartjs for the gantt chart is causing too many headaches. I wonder if it would be better to just rip it all out. But that might have to be for 3.2 |
| if (hoveredData?.taskId !== undefined) { | ||
| setTooltipData({ | ||
| taskId: hoveredData.taskId, | ||
| visible: true, |
There was a problem hiding this comment.
Do we need visible or can we just check that taskId, x, y exist?
There was a problem hiding this comment.
it seems redundant. I've removed it.
| data={data} | ||
| selectedTimezone={selectedTimezone} | ||
| taskId={tooltipData.taskId} | ||
| translate={translate} |
There was a problem hiding this comment.
Why are we passing translate instead of calling useTranslate inside of GanttTooltip?
There was a problem hiding this comment.
I try to make it more efficient without initializing it again. But it would be more straightforward to call it inside the component. I've updated it.
| const [lightBg, darkBg, lightBorder, darkBorder, lightText, darkText] = useToken("colors", [ | ||
| "white", | ||
| "gray.800", | ||
| "gray.200", | ||
| "gray.600", | ||
| "gray.800", | ||
| "white", | ||
| ]); |
There was a problem hiding this comment.
Let's use the semantic token colors directly in <Box />. We only need useToken when we need to pass the actual hex code to a non chakra component.
There was a problem hiding this comment.
Colors don't match our existing tooltips either.
There was a problem hiding this comment.
Thanks for catching this. I've reversed the style between dark and light.
|
I haven't looked too deeply, but the hover interaction feels slower. I wonder if the state change is forcing extra rendering |
guan404ming
force-pushed
the
fix-tooltip
branch
from
8774364 to
94c3699
Compare
I agree, I also think it might be better to slowly move away from Chart.js. We can take it step by step, maybe target 3.2 or 3.1.x.
yes, the hover feels noticeably slower. Probably because extra re-rendering like you said. This part is kind of tricky since we don’t have direct control over the bar element (it is also a headache part), so i think it’ll take some testing and improvement to smooth things out. |
|
Let's save this change for when we try to remove chartjs from the gantt chart entirely instead of trying to hack something together. |
|
Close this one for now since rewrite the chart seems like a better option for this issue but could re-open if needed in the future. |
2 participants
Related
#51667
Why
content visibility
How
Screen.Recording.2025-09-23.at.10.41.13.PM.mov
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named
{pr_number}.significant.rstor{issue_number}.significant.rst, in airflow-core/newsfragments.